-
Notifications
You must be signed in to change notification settings - Fork 64
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Environment: check GD and Imagick availability & supported formats. #134
Environment: check GD and Imagick availability & supported formats. #134
Conversation
Thanks so much! I haven't had the chance to test yet, but this looks very close. Due to #111 , unfortunately, this needs to be updated in two places at the moment -- both I need to resurrect that issue / patch / diff at some point (or help is welcome) to refactor and turn it into an optional SSH call for gathering environment data, but I think it's fine to go ahead and add this in the meantime. |
I missed that, will add there as well. Thanks for the tip. |
Addressed feedback:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
@getsource friendly ping, this should be ready to go! |
Thanks! Trying to test, but ran into some environment difficulties with app passwords and the reporter plugin. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested, and the information gets uploaded to the reporter as expected.
A couple of notes:
- We'll need to add something to the reporter to display this information (Edit: This does not have to happen before this gets merged, but we should probably open an Issue either now, or when it does).
- When successful, an array gets inserted for both items. Would an empty array (or not including it at all / "empty") make it easier to parse this in the reporter when "unavailable"?
Here are relevant locations for the reporter, for consideration of the unavailable values: |
I created an issue to add the data if available to the reports: WordPress/phpunit-test-reporter#97
Good point, I hadn't considered the reporting end of things. Makes more sense for the default value to be an empty array vs. the string 'unavailable'. I will update the PR. |
@getsource - I changed the defaults to empty array as per your suggestion. This should be good to merge. |
@pfefferle @getsource What are the next steps to get this merged (plus WordPress/phpunit-test-reporter#98). Also: once this is merged, how is deployed in the wild, when could we hope to start seeing data reported? |
My suggested steps would be:
I don't think this PR necessarily needs to wait for the latter, but having them work together as a proof of concept definitely would help verify/validate. I don't recall whether sending the data to the reporter when it isn't expecting it (if this PR is merged first) would cause validation issues with the API or not, but looking+testing to make sure it works as expected would be good before this is merged. As far as when data is received: After the above is done, because updating the runner code is dependent on the client, I suspect some hosts will start to report right away (if they clone this repo as part of the testing process), and for others it will take an undefined amount of time until they decide to manually update. I don't have the time at the moment to work on this, unfortunately, so help is appreciated. |
Just cross referencing the recent Core changeset that added this data to update pings to .org: https://core.trac.wordpress.org/changeset/53753. |
Addresses #131.